Skip to content

Add regression test for issue #1027 #1069

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

felixweinberger
Copy link
Contributor

@felixweinberger felixweinberger commented Jul 1, 2025

Motivation and Context

Issue #1027 reports that MCP server cleanup code (after yield in lifespan) is unreachable when the process is terminated. This PR adds tests that:

  1. Demonstrate the issue exists on all platforms (not just Windows)
  2. Verify that PR fweinberger/align shutdown with spec #1091's approach of closing stdin first would fix the issue

How Has This Been Tested?

Added two tests in test_1027_win_unreachable_cleanup.py:

  • test_lifespan_cleanup_executed - Shows cleanup doesn't run with current termination (xfails)
  • test_stdin_close_triggers_cleanup - Shows cleanup works when stdin is closed first (passes)

Tested on both macOS and Windows.

When run with PR #1091's changes, both tests pass.

Breaking Changes

None. Tests only.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@felixweinberger felixweinberger changed the base branch from main to fweinberger/stream-cleanup-only-approach July 1, 2025 20:29
@felixweinberger felixweinberger force-pushed the fweinberger/issue-1027 branch from 0e4626a to c032dbc Compare July 1, 2025 20:30
@felixweinberger felixweinberger marked this pull request as ready for review July 1, 2025 20:35
@felixweinberger felixweinberger force-pushed the fweinberger/issue-1027 branch 4 times, most recently from 1224de0 to 3c8eabe Compare July 1, 2025 20:48
@felixweinberger felixweinberger force-pushed the fweinberger/stream-cleanup-only-approach branch 12 times, most recently from 7af9e65 to c9b43bc Compare July 7, 2025 13:59
@felixweinberger felixweinberger force-pushed the fweinberger/issue-1027 branch from 3c8eabe to 72cfeca Compare July 7, 2025 15:03
@felixweinberger felixweinberger marked this pull request as draft July 7, 2025 15:03
Base automatically changed from fweinberger/stream-cleanup-only-approach to main July 8, 2025 13:57
@felixweinberger felixweinberger force-pushed the fweinberger/issue-1027 branch from 72cfeca to 526d276 Compare July 8, 2025 16:39
@felixweinberger felixweinberger changed the base branch from main to fweinberger/align-shutdown-with-spec July 8, 2025 16:39
@felixweinberger felixweinberger force-pushed the fweinberger/issue-1027 branch 5 times, most recently from 79ebf22 to 34643bd Compare July 8, 2025 19:35
This test shows that MCP server cleanup code in lifespan doesn't run
when the process is terminated, but does run when stdin is closed first
(as implemented in PR #1044).

The test includes:
- Demonstration of current broken behavior (cleanup doesn't run)
- Verification that stdin closure allows graceful shutdown
- Windows-specific ResourceWarning handling
- Detailed documentation of the issue and solution

Github-Issue:#1027
@felixweinberger felixweinberger force-pushed the fweinberger/issue-1027 branch from 34643bd to 26e1e49 Compare July 8, 2025 19:44
@felixweinberger felixweinberger marked this pull request as ready for review July 8, 2025 19:48
Copy link
Contributor

@bhosmer-ant bhosmer-ant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, LGTM!

@bhosmer-ant bhosmer-ant merged commit 96b4059 into fweinberger/align-shutdown-with-spec Jul 8, 2025
10 checks passed
@bhosmer-ant bhosmer-ant deleted the fweinberger/issue-1027 branch July 8, 2025 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants